Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

⚠️ Make ip-address-manager an IPAM provider for CAPI #692

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

⚠️ Make ip-address-manager an IPAM provider for CAPI #692

wants to merge 1 commit into from

Conversation

peppi-lotta
Copy link
Member

@peppi-lotta peppi-lotta commented Sep 18, 2024 •
edited
Loading

This commit makes it possible to:

  • Deploy IPAM with clusterctl
  • Reconsile CAPI's ipaddressclaims with this managers ippools

I have two PR's, one in metal-dev-env and one in CAPM3 to mach these changes

All three PRs' changes need to be tested together. Check test in the metal-dev-env PR.

What this PR does / why we need it: To make this IPAM into a provider for CAPI

@metal3-io-bot metal3-io-bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 18, 2024
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 7 times, most recently from 4fcd34f to abe0f52 Compare September 19, 2024 09:01
@peppi-lotta
Copy link
Member Author

/test metal3-ubuntu-e2e-integration-test-main
/test metal3-centos-e2e-integration-test-main

@peppi-lotta peppi-lotta mentioned this pull request Sep 19, 2024
Open
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 2 times, most recently from 32d5986 to 2f06f8b Compare September 26, 2024 08:15
@peppi-lotta peppi-lotta mentioned this pull request Oct 2, 2024
Open
Rozzii
Rozzii previously requested changes Oct 22, 2024
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@peppi-lotta this is quite a big change so I prefer to review after all the CI checks are green.

Please fix the markdown and run the tests, then ping me please if everything is green.

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 5 times, most recently from 5fa145b to b610263 Compare October 24, 2024 07:02
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 2 times, most recently from 6b82fe0 to 8e486a5 Compare October 31, 2024 08:00
lentzi90
lentzi90 reviewed Nov 1, 2024
View reviewed changes
Copy link
Member

@lentzi90 lentzi90 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great job!
We could probably use generics to avoid the double functions for everything. However, I think it is good to keep it like this for now since it means we know that the original code did not change.
Just minor things below about naming.

config/default/kustomization.yaml Outdated Show resolved Hide resolved
config/default/kustomization.yaml Outdated Show resolved Hide resolved
metadata.yaml Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
ipam/ippool_manager.go Outdated Show resolved Hide resolved
@Rozzii Rozzii modified the milestone: IPAM - v1.9 Nov 6, 2024
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 4 times, most recently from 027c714 to 1cb7909 Compare November 7, 2024 11:23
tuminoid
tuminoid reviewed Nov 13, 2024
View reviewed changes
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM besides tiniest nits.

metadata.yaml Outdated Show resolved Hide resolved
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 4 times, most recently from a4fbb0b to fe5bc60 Compare November 15, 2024 11:10
@tuminoid
Copy link
Member

I think this is pretty much done, so let's merge it when 1.9 is cut to get maximum amount of testing in 1.10 cycle.

Rozzii
Rozzii reviewed Nov 16, 2024
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Nov 16, 2024
@metal3-io-bot metal3-io-bot added the needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2025
@mquhuy
Copy link
Member

mquhuy commented Jan 23, 2025

@peppi-lotta this needs rebase

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch from fe5bc60 to 2514136 Compare January 27, 2025 11:31
@metal3-io-bot metal3-io-bot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates that a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 27, 2025
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch 3 times, most recently from 4230995 to 8b5afac Compare January 27, 2025 14:25
@Rozzii
Copy link
Member

Rozzii commented Jan 28, 2025

/test metal3-centos-e2e-integration-test-main
/test metal3-ubuntu-e2e-integration-test-main

Rozzii
Rozzii reviewed Jan 28, 2025
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2025
@peppi-lotta
Copy link
Member Author

/test metal3-centos-e2e-integration-test-main /test metal3-ubuntu-e2e-integration-test-main

These are going to fail at cluster creation. This needs test override to get in.

@peppi-lotta
Copy link
Member Author

peppi-lotta commented Jan 28, 2025 •
edited
Loading

Here is fullstack test with all the appropriate branches: https://jenkins.nordix.org/view/Metal3/job/metal3-fullstack-build/4/

@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch from 8b5afac to 281aaf6 Compare January 28, 2025 13:12
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 28, 2025
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch from 281aaf6 to 4fc8633 Compare January 28, 2025 13:27
@peppi-lotta
This commit makes it possible to:
346d9eb 
- Deploy IPAM with clusterctl
- Reconsile CAPI's ipaddressclaims with this managers ippools

Signed-off-by: peppi-lotta <[email protected]>
@peppi-lotta peppi-lotta force-pushed the peppi-lotta/make-ipam-a-provider-for-capi branch from 4fc8633 to 346d9eb Compare January 29, 2025 07:20
@Rozzii
Copy link
Member

Rozzii commented Feb 3, 2025

/override metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main
As requested.

@metal3-io-bot
Copy link
Contributor

@Rozzii: Overrode contexts on behalf of Rozzii: metal3-centos-e2e-integration-test-main, metal3-ubuntu-e2e-integration-test-main

In response to this:

/override metal3-centos-e2e-integration-test-main metal3-ubuntu-e2e-integration-test-main
As requested.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

Rozzii
Rozzii reviewed Feb 3, 2025
View reviewed changes
Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Feb 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Rozzii Rozzii Rozzii left review comments

@lentzi90 lentzi90 lentzi90 left review comments

@tuminoid tuminoid tuminoid left review comments

@smoshiur1237 smoshiur1237 Awaiting requested review from smoshiur1237

Assignees
No one assigned
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Metal3 - Roadmap
Status: IPAM WIP
Milestone
IPAM - v1.10
Development

Successfully merging this pull request may close these issues.
6 participants
@peppi-lotta @Rozzii @metal3-io-bot @tuminoid @mquhuy @lentzi90